Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

build: link libatomic on gcc or clang on linux #28532

Closed
wants to merge 1 commit into from
Closed

build: link libatomic on gcc or clang on linux #28532

wants to merge 1 commit into from
+1 −1

Conversation

marsam
Copy link

@marsam marsam commented Jul 4, 2019

See: #28231 #28232
libatomic is a gcc library. Clang default is to be built to use it on Linux.
cc: @devsnek @addaleax

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 4, 2019
@Trott
Copy link
Member

Trott commented Jul 4, 2019

@nodejs/node-gyp

@bnoordhuis
Copy link
Member

node-gyp only uses common.gypi so this change should be harmless in that respect.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/24273/

sam-github
sam-github reviewed Jul 4, 2019
View reviewed changes
node.gyp Outdated
@@ -289,7 +289,7 @@
'-Wl,-bnoerrmsg',
],
}],
['(OS=="linux" or OS=="mac") and llvm_version!=0', {
['llvm_version==0 or (llvm_version!=0 and OS=="linux")', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble understanding what problem this fixes. This is adding -latomic for all platforms when gcc is used, why? It is also removing -latomic on Macs when clang is used, but wasn't one of the bugs in the linked reports explicitly about needing -latomic with clang on Mac?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically clang always needs libatomic, but node only uses it on mac and linux for the v8 sigsegv handlers.

I don't believe it's ever needed on GCC, but I could be wrong.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is adding -latomic for all platforms when gcc is used, why?

I assumed this was implicitly required by the previous state. I've replace it with:

OS=="linux" and llvm_version!=0

to only add it on cllang+linux

but wasn't one of the bugs in the linked reports explicitly about needing -latomic with clang on Mac?

no, the #28231 mention clang+linux, it fails on clang+darwin NixOS/nixpkgs#64253 (comment)

technically clang always needs libatomic

not quite, libatomic is a gcc library. Clang by default is to be built to use it on Linux, and to use compiler-rt on darwin

devsnek
devsnek reviewed Jul 5, 2019
View reviewed changes
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will fail on mac+clang now

@marsam
Copy link
Author

marsam commented Jul 5, 2019 •
edited
Loading

@devsnek I've tested on clang+darwin https://hatebin.com/vrlbbcwsia and gcc+linux (both with nix)
can you tell me which versions are you using to build it? and which is the output error?

@devsnek
Copy link
Member

devsnek commented Jul 5, 2019 •
edited
Loading

@marsam on Apple LLVM version 10.0.0 (clang-1000.10.25.5) (what macos ships with) the build will fail if -latomic is not passed.

example error:

out/Release/obj.target/libnode/src/node.o: In function `node::RegisterSignalHandler(int, void (*)(int, siginfo_t*, void*), bool)':
node.cc:(.text+0x17b4): undefined reference to `__atomic_is_lock_free'

@marsam
Copy link
Author

marsam commented Jul 5, 2019

AFAIK __atomic_is_lock_free on darwin should be provided by clang compiler-rt. What occurs me it that you might be using libgcc_s, from clang docs:

Clang does not currently automatically link against libatomic when using libgcc_s. You may need to manually add -latomic to support this configuration when using non-native atomic operations (if you see link errors referring to _atomic* functions).

I think adding -latomic in darwin regarless, is a bad choice, because only solves for that specific case. For instance krita disables libatomic on darwin, so does chapel-lang. I haven't found a project which by default links libatomic on darwin unless it explicitly requires gcc.

@devsnek
Copy link
Member

devsnek commented Jul 5, 2019

I don't really know what any of the stuff you linked means, but you have an alternative to -latomic that doesn't break macos+clang, please include it in this pr.

@BridgeAR
Copy link
Member

Ping @marsam

@marsam
Copy link
Author

marsam commented Jul 19, 2019

@BridgeAR sorry for the delay, I'm not sure how to solve @devsnek's issue.
Without this patch, nodejs is basically broken on darwin, because darwin does not have libatomic.

I'm not familiar with gyp, and I could patch it downstream, hence I will not be working on this PR any further. Feel free to close it, thanks.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/24566/

@ryandesign ryandesign mentioned this pull request Oct 23, 2019
Closed
@marsam
Copy link
Author

marsam commented Nov 19, 2019

Closed in favor of #30099

@marsam marsam closed this Nov 19, 2019
@marsam marsam deleted the fix-atomic-linkage-clang branch November 19, 2019 11:53
@marv marv mentioned this pull request Nov 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sam-github sam-github sam-github left review comments

@devsnek devsnek devsnek left review comments

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@marsam @Trott @bnoordhuis @nodejs-github-bot @devsnek @BridgeAR @sam-github